nsenter: set {uid,gid} explicitly around namespace creation #975
Conversation
|
|
|
|
||
| /* Cache the user namespace entry. */ | ||
| if (ns->ns == CLONE_NEWUSER || !strcmp(namespace, "user")) | ||
| userns = ns; |
There was a problem hiding this comment.
When ns->ns == CLONE_NEWUSER, the !strcmp(namespace, "user") must be true.
So, this code maybe simplify as following:
if (ns->ns == CLONE_NEWUSER)
userns = ns;
There was a problem hiding this comment.
IIRC I wrote it like this to avoid issues where CLONE_NEWUSER is not defined by glibc (so nsflags would return 0). Though I might have fixed that in the meantime, since CLONE_NEWUSER is required by a bunch of other things. I'll take a look later.
|
This has been rebased. @avagin can you double-check that this fix still is fine, since I had to un-split the /cc @opencontainers/runtime-spec-maintainers |
|
LGTM |
|
unshare fails with EPERM on RHEL 7. |
| if (setresgid(config.hostgid, config.hostgid, config.hostgid) < 0) | ||
| bail("failed to set gid to host"); | ||
| if (setresuid(config.hostuid, config.hostuid, config.hostuid) < 0) | ||
| bail("failed to set uid to host"); |
There was a problem hiding this comment.
@mrunalp I guess this is what's causing the failure. The problem is that this is the fix for the security issue (a racing ptrace can cause root privilege escalation inside a container). I'm starting to think that the issues here are on the SELinux side and not on the runC side...
There was a problem hiding this comment.
@cyphar I'll look closer but it looked like the split of unshare or NEWUSER and the other cloneflags.
There was a problem hiding this comment.
Recently, I read an email on a kernel mailing list about "safely creating container processes" and the process for doing it safely is actually quite a bit more complicated than what we're doing now. Essentially the method requires creating a "sacrifical" user namespace before we create all of the actual container namespaces (you unshare(CLONE_NEWUSER) twice). The problem is that because of SELinux we simply can't implement that -- which IMO is a serious problem.
I'll see if I can find the email. My point is that I feel like the issues are on the kernel side of things, because I'm trying to make this process safer and SELinux is having problems with it. If worse comes to worst we can have SELinux-specific code here that changes how we create containers -- it won't be pretty but at least container creation will be safer without breaking SELinux support.
In general, it is a bad idea to be unmapped inside a user namespace at
any point (especially when euid=[kuid 0]) as it can lead to security
vulnerabilities. Also, in certain SELinux setups you must also be mapped
in your user namespace when unsharing other namespaces.
Deal with all of this by parsing the {uid,gid}maps and then setresuid(2)
to the right user before and after the critical unshare(CLONE_NEWUSER)
(as well as dealing with setns(2) by changing user to the owner of the
namespace file we're joining).
Fixes: CVE-2015-8709
Reported-by: Andrey Vagin <avagin@virtuozzo.com>
Reported-by: Mrunal Patel <mpatel@redhat.com>
Signed-off-by: Aleksa Sarai <asarai@suse.de>
|
@cyphar can you rebase this change if still relevant ? |
|
Closing since #1562 takes care of this. Just cleaning up the milestone some. |
In general, it is a bad idea to be unmapped inside a user namespace at
any point (especially when euid=[kuid 0]) as it can lead to security
vulnerabilities. Also, in certain SELinux setups you must also be mapped
in your user namespace when unsharing other namespaces.
Deal with all of this by parsing the {uid,gid}maps and then setresuid(2)
to the right user before and after the critical unshare(CLONE_NEWUSER)
(as well as dealing with setns(2) by changing user to the owner of the
namespace file we're joining).
Fixes: CVE-2015-8709
Reported-by: Andrey Vagin avagin@virtuozzo.com
Reported-by: Mrunal Patel mpatel@redhat.com
Signed-off-by: Aleksa Sarai asarai@suse.de
Fixes #959.
Note: This is based on
#950and #977.